Skip to content

Conversation

royitaqi
Copy link
Contributor

@royitaqi royitaqi commented Sep 19, 2025

Motiviation

This helps the development of the lldb-dap binary. For example, when testing a locally built lldb-dap binary, one probably wants to restart the server after each build in order to use the latest binary. Not doing so leads to confusion like "why the new lldb-dap isn't doing what I just changed?".

This patch adds dependency to the chokidar package. The reason is that we need something to detect changes to the lldb-dap binary file and chokidar appears to be the most reliable package to do so. An alternative solution which doesn't require adding dependencies is discussed below (solution 1).

Two different solutions considered

Solution 1: Restart server when lldb-dap binary's modification time changes. #159481 implements solution 1.

Solution 2: Restart server when lldb-dap binary has changed (as detected by a file system watcher). This patch implements solution 2 (using chokidar).

This patch (solution 2)

If the lldb-dap binary has changed, the next time the user start a debug session, a dialog box will show up and prompt the user to restart the server. Depend on what has changed, the dialog box will show different content (see below)

  • When both the lldb-dap binary and the arguments have changed:
diff_args_and_binary
  • When only the lldb-dap binary has changed:
diff_binary
  • When only the arguments have changed (existing):
diff_args

@llvmbot
Copy link
Member

llvmbot commented Sep 19, 2025

@llvm/pr-subscribers-lldb

Author: Roy Shi (royitaqi)

Changes

Motiviation

This helps the development of the lldb-dap binary. For example, when testing a locally built lldb-dap binary, one probably wants to restart the server after each build in order to use the latest binary. Not doing so leads to confusion like "why the new lldb-dap isn't doing what I just changed?".

Two different solutions considered

Solution 1: Restart server when lldb-dap binary's modification time changes. #159481 implements solution 1.

Solution 2: Restart server when lldb-dap binary has changed (as detected by vscode.workspace.createFileSystemWatcher). This patch implements solution 2.

This patch (solution 2)

If the lldb-dap binary has changed, the next time the user start a debug session, a dialog box will show up and prompt the user to restart the server.

<img width="260" height="384" alt="Screenshot 2025-09-19 at 8 40 31 AM" src="https://github.com/user-attachments/assets/543947ce-fe30-4850-84a7-28009f7cd3d6" />

Note that the message and detail of the dialog box is different from the existing one (which is shown when lldb-dap path/args/env has changed), shown below.

<img width="521" height="358" alt="Screenshot 2025-09-19 at 8 42 34 AM" src="https://github.com/user-attachments/assets/fa926302-c1e9-45e2-84ad-70fbd16d8dca" />


Full diff: https://github.com/llvm/llvm-project/pull/159797.diff

1 Files Affected:

  • (modified) lldb/tools/lldb-dap/src-ts/lldb-dap-server.ts (+64-13)
diff --git a/lldb/tools/lldb-dap/src-ts/lldb-dap-server.ts b/lldb/tools/lldb-dap/src-ts/lldb-dap-server.ts
index 774be50053a17..7a1754f4bce6a 100644
--- a/lldb/tools/lldb-dap/src-ts/lldb-dap-server.ts
+++ b/lldb/tools/lldb-dap/src-ts/lldb-dap-server.ts
@@ -1,4 +1,5 @@
 import * as child_process from "node:child_process";
+import * as path from "path";
 import { isDeepStrictEqual } from "util";
 import * as vscode from "vscode";
 
@@ -12,6 +13,10 @@ export class LLDBDapServer implements vscode.Disposable {
   private serverProcess?: child_process.ChildProcessWithoutNullStreams;
   private serverInfo?: Promise<{ host: string; port: number }>;
   private serverSpawnInfo?: string[];
+  // Detects changes to the lldb-dap executable file since the server's startup.
+  private serverFileWatcher?: vscode.FileSystemWatcher;
+  // Indicates whether the lldb-dap executable file has changed since the server's startup.
+  private serverFileChanged?: boolean;
 
   constructor() {
     vscode.commands.registerCommand(
@@ -83,6 +88,18 @@ export class LLDBDapServer implements vscode.Disposable {
       });
       this.serverProcess = process;
       this.serverSpawnInfo = this.getSpawnInfo(dapPath, dapArgs, options?.env);
+      this.serverFileChanged = false;
+      // Cannot do `createFileSystemWatcher(dapPath)` for a single file. Have to use `RelativePattern`.
+      // See https://github.com/microsoft/vscode/issues/141011#issuecomment-1016772527
+      this.serverFileWatcher = vscode.workspace.createFileSystemWatcher(
+        new vscode.RelativePattern(
+          vscode.Uri.file(path.dirname(dapPath)),
+          path.basename(dapPath),
+        ),
+      );
+      this.serverFileWatcher.onDidChange(() => {
+        this.serverFileChanged = true;
+      });
     });
     return this.serverInfo;
   }
@@ -100,20 +117,34 @@ export class LLDBDapServer implements vscode.Disposable {
     args: string[],
     env: NodeJS.ProcessEnv | { [key: string]: string } | undefined,
   ): Promise<boolean> {
-    if (!this.serverProcess || !this.serverInfo || !this.serverSpawnInfo) {
+    if (
+      !this.serverProcess ||
+      !this.serverInfo ||
+      !this.serverSpawnInfo ||
+      !this.serverFileWatcher ||
+      this.serverFileChanged === undefined
+    ) {
       return true;
     }
 
-    const newSpawnInfo = this.getSpawnInfo(dapPath, args, env);
-    if (isDeepStrictEqual(this.serverSpawnInfo, newSpawnInfo)) {
-      return true;
-    }
+    // Check if the server has changed. If so, generate message and detail for user prompt.
+    const messageAndDetail = (() => {
+      if (this.serverFileChanged) {
+        return {
+          message:
+            "The lldb-dap binary has changed. Would you like to restart the server?",
+          detail: `An existing lldb-dap server (${this.serverProcess.pid}) is running with an old binary.
 
-    const userInput = await vscode.window.showInformationMessage(
-      "The arguments to lldb-dap have changed. Would you like to restart the server?",
-      {
-        modal: true,
-        detail: `An existing lldb-dap server (${this.serverProcess.pid}) is running with different arguments.
+Restarting the server will interrupt any existing debug sessions and start a new server.`,
+        };
+      }
+
+      const newSpawnInfo = this.getSpawnInfo(dapPath, args, env);
+      if (!isDeepStrictEqual(this.serverSpawnInfo, newSpawnInfo)) {
+        return {
+          message:
+            "The arguments to lldb-dap have changed. Would you like to restart the server?",
+          detail: `An existing lldb-dap server (${this.serverProcess.pid}) is running with different arguments.
 
 The previous lldb-dap server was started with:
 
@@ -124,15 +155,31 @@ The new lldb-dap server will be started with:
 ${newSpawnInfo.join(" ")}
 
 Restarting the server will interrupt any existing debug sessions and start a new server.`,
+        };
+      }
+
+      return null;
+    })();
+
+    // If the server hasn't changed, continue startup without killing it.
+    if (messageAndDetail === null) {
+      return true;
+    }
+
+    // The server has changed. Prompt the user to restart it.
+    const { message, detail } = messageAndDetail;
+    const userInput = await vscode.window.showInformationMessage(
+      message,
+      {
+        modal: true,
+        detail,
       },
       "Restart",
       "Use Existing",
     );
     switch (userInput) {
       case "Restart":
-        this.serverProcess.kill();
-        this.serverProcess = undefined;
-        this.serverInfo = undefined;
+        this.dispose();
         return true;
       case "Use Existing":
         return true;
@@ -156,6 +203,10 @@ Restarting the server will interrupt any existing debug sessions and start a new
     if (this.serverProcess === process) {
       this.serverProcess = undefined;
       this.serverInfo = undefined;
+      this.serverSpawnInfo = undefined;
+      this.serverFileWatcher?.dispose();
+      this.serverFileWatcher = undefined;
+      this.serverFileChanged = undefined;
     }
   }
 

});
this.serverProcess = process;
this.serverSpawnInfo = this.getSpawnInfo(dapPath, dapArgs, options?.env);
this.serverFileChanged = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

@royitaqi royitaqi Sep 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to clarify: vscode.workspace.createFileSystemWatcher now works on my machine. Are you suggesting to use chokidar instead?

Also, even with chokidar, I will have to have this.serverFileChanged, so that we know that the lldb-dap binary has changed at the start of the next debug session. Make sense?

Updated the patch to use chokidar.

@royitaqi
Copy link
Contributor Author

@walter-erquinigo: Updated the patch to use chokidar. It worked out of the box, which is nice~

Any other suggestions from you?

@royitaqi
Copy link
Contributor Author

royitaqi commented Sep 22, 2025

@walter-erquinigo: Gentle ping for a review.

Since your last review, I have:

  • Used chokidar.
  • Rebased on latest main.
  • When both the lldb-dap binary and the arguments have changed, I made the dialog box show both changes (see the first screenshot in the patch's description).

@JDevlieghere
Copy link
Member

When you merge this, please include a little bit of motivation for introducing a new external dependency. I skimmed the package and it seems well-established with limited transitive dependencies, but generally speaking we should have a pretty high bar for adding new dependencies.

@royitaqi
Copy link
Contributor Author

royitaqi commented Sep 22, 2025

When you merge this, please include a little bit of motivation for introducing a new external dependency.

I have just updated the "Motivation" section of the patch description to include an explanation. Feel free to LMK if you want me to change anything there.

I skimmed the package and it seems well-established with limited transitive dependencies, but generally speaking we should have a pretty high bar for adding new dependencies.

If we want no/less dependency, we can use #159481 instead. I can update it so that it only uses the fs package (which IIUC should be included by default). However, that solution uses a different approach (checks the modification time of the lldb-dap binary) and is considered less reliable per @walter-erquinigo's comment (I'm still yet to learn why that is the case).

Copy link
Member

@walter-erquinigo walter-erquinigo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool! thanks!

About the reliability, all I can tell you is that I used to manage a few vscode extensions and tracking modification times was not always working on all systems. At least that's why my users reported, but I never got access to those systems so I couldn't really know the underlying issues. But when I switched to using well established libraries for this, my problems went away...

@royitaqi
Copy link
Contributor Author

@walter-erquinigo: Thanks for sharing the context about the reliability. I appreciate it. Will keep that in mind.

@royitaqi royitaqi merged commit 6007a4d into llvm:main Sep 23, 2025
9 checks passed
@z2oh
Copy link
Contributor

z2oh commented Sep 24, 2025

I started hitting an error about missing chokidar module today and came across this PR when searching. My error is:

2025-09-24 10:57:12.237 [info] ExtensionService#_doActivateExtension llvm-vs-code-extensions.lldb-dap, startup: false, activationEvent: 'onLanguage:swift', root cause: swiftlang.swift-vscode
2025-09-24 10:57:12.240 [error] Activating extension llvm-vs-code-extensions.lldb-dap failed due to an error:
2025-09-24 10:57:12.240 [error] Error: Cannot find module 'chokidar'
Require stack:
- /Users/jeremy/.vscode/extensions/llvm-vs-code-extensions.lldb-dap-0.2.17/out/lldb-dap-server.js
- /Users/jeremy/.vscode/extensions/llvm-vs-code-extensions.lldb-dap-0.2.17/out/extension.js
- /Applications/Visual Studio Code.app/Contents/Resources/app/out/vs/workbench/api/node/extensionHostProcess.js

I'm not too familiar with the node ecosystem, but is there some missing step here that should be distributing or installing chokidar? Reinstalling the extension didn't help, but manually running npm install in /Users/jeremy/.vscode/extensions/llvm-vs-code-extensions.lldb-dap-0.2.17 pulls down this dependency and the extension can start.

(cc @JDevlieghere ; unless I'm just holding it wrong I think ldb-dap-0.2.17 is broken)

@JDevlieghere
Copy link
Member

Thanks, fixed by #160598

@royitaqi
Copy link
Contributor Author

royitaqi commented Sep 24, 2025

Sorry for the trouble, guys~

@matthewbastien, @JDevlieghere: Thanks for the quick fix. I appreciate it~!

JDevlieghere pushed a commit to swiftlang/llvm-project that referenced this pull request Oct 14, 2025
…59797)

# Motiviation

This helps the development of the lldb-dap binary. For example, when
testing a locally built lldb-dap binary, one probably wants to restart
the server after each build in order to use the latest binary. Not doing
so leads to confusion like "why the new lldb-dap isn't doing what I just
changed?".

This patch adds dependency to the `chokidar` package. The reason is that
we need something to detect changes to the `lldb-dap` binary file and
`chokidar` appears to be the most reliable package to do so. An
alternative solution which doesn't require adding dependencies is
discussed below (solution 1).

# Two different solutions considered

Solution 1: Restart server when lldb-dap binary's modification time
changes. llvm#159481 implements
solution 1.

Solution 2: Restart server when lldb-dap binary has changed (as detected
by a file system watcher). This patch implements solution 2 (using
`chokidar`).

# This patch (solution 2)

If the lldb-dap binary has changed, the next time the user start a debug
session, a dialog box will show up and prompt the user to restart the
server. Depend on what has changed, the dialog box will show different
content (see below)

* When both the lldb-dap binary and the arguments have changed:
<img width="520" height="357" alt="diff_args_and_binary"
src="https://github.com/user-attachments/assets/6580e05f-05c3-4d80-8c1a-9c3abf279218"
/>

* When only the lldb-dap binary has changed:
<img width="260" height="384" alt="diff_binary"
src="https://github.com/user-attachments/assets/933b8987-9c21-44f3-ad68-57b8eeb58525"
/>

* When only the arguments have changed (existing):
<img width="520" height="343" alt="diff_args"
src="https://github.com/user-attachments/assets/c0e6771c-ad32-4fb8-b5df-b34cce48ed1a"
/>

(cherry picked from commit 6007a4d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants